Repository navigation
feat: implement pausable streams in SuperGoodDollar contract - #303
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="contracts/token/superfluid/SuperGoodDollar.sol" line_range="132" />
<code_context>
+ // the CFA packs int96 flowRate at bits [128,224) of the first word
+ bool increased;
+ assembly {
+ let newRate := signextend(11, shr(128, calldataload(data.offset)))
+ let oldRate := signextend(11, shr(128, sload(slot)))
+ increased := sgt(newRate, oldRate)
+ }
+ if (increased) revert SUPER_GOODDOLLAR_PAUSED();
</code_context>
<issue_to_address>
**issue (bug_risk):** `oldRate` is loaded from `sload(slot)`, but `FixedSizeData.storeData` stores the array length at `slot` and stores the first agreement-data word at `slot + 1`. For an existing CFA flow, `oldRate` is therefore the data length (normally `2`), so decreasing a normal positive flow to another positive rate is treated as an increase and reverts while paused.
**Triggers:** When an existing flow is decreased, rather than closed, while the token is paused.
**Suggested fix:** Load the first stored agreement-data word with `sload(add(slot, 1))` before extracting the packed flow rate.
```suggestion
let oldRate := signextend(11, shr(128, sload(add(slot, 1))))
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and while paused, this changes whether Superfluid can create or increase token streams, and a faulty calldata/storage decoding or agreement-slot assumption could either leave transfers running during an incident or block needed flows. Reverting restores the gate, but any tokens transferred by an incorrectly permitted stream may not be recoverable.
Blocking findings: contracts/token/superfluid/SuperGoodDollar.sol:132
There was a problem hiding this comment.
🟡 Changes recommended
The new pause guard decodes newRate incorrectly because it reads the calldata array length instead of data[0], so it won’t reliably block opening/increasing streams.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes SuperGoodDollar’s pause enforcement for Superfluid Constant Flow Agreements (CFA) by moving the pause guard onto the updateAgreementData path that CFA actually uses, while preserving the ability to decrease/close/liquidate existing streams during an incident.
Changes:
- Add a
SuperfluidTokenhook (_beforeAgreementDataUpdate) invoked fromupdateAgreementDatabefore writing agreement data. - Implement pause-aware CFA flow-rate gating in
SuperGoodDollarby comparing the newly written flow rate vs the stored one and rejecting only increases while paused. - Add tests to ensure new streams and stream increases revert while paused, but closing streams remains possible.
File summaries
| File | Description |
|---|---|
| test/token/SuperGoodDollar.test.ts | Adds regression tests for pausing behavior across create/update/delete flow operations. |
| contracts/token/superfluid/SuperGoodDollar.sol | Implements the pause guard on agreement-data updates by decoding and comparing flow rates. |
| contracts/token/superfluid/SuperfluidToken.sol | Introduces a reusable pre-write hook for updateAgreementData to enable token-specific gating. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
sirpy
left a comment
sirpy
left a comment
There was a problem hiding this comment.
Can the DAO also have permissions to delete a flow?
…reverify schedule test
|
@sirpy |
…ate errors" This reverts commit 3926686.
…en for SuperGoodDollar compatibility
…y of getAgreementData in SuperfluidToken
hellwolf
left a comment
hellwolf
left a comment
There was a problem hiding this comment.
Thank you! Looks good to me now.
I'd recommend keep tracking the upstream code when possible.
|
@blueogin can we move the new flow rate helper methods into an external library to reduce the size? |
|
@sirpy |
…ogic and update SuperGoodDollar to utilize it
|
@sirpy |
sirpy
left a comment
sirpy
left a comment
There was a problem hiding this comment.
commit the library and make sure tests are passing
|
@sirpy |
Description
pause()did not stop streaming. The guard sat oncreateAgreement, which the CFA never calls when a flow is opened — it writes all flow state throughupdateAgreementData. Anyone could open, increase, or keep running a G$ stream while the token was paused.This moves the guard to the path the CFA actually uses, and gates it so that opening or increasing a flow is blocked while paused, but closing, decreasing and liquidating stay available.
The bug
Verified against the installed
@superfluid-finance/ethereum-contracts@1.8.1.ConstantFlowAgreementV1calls, on the token:updateAgreementData— for create, update and delete (line 1402, plus the ACL paths at 862/1079)updateAgreementStateSlot— account flow state (line 1104)settleBalance— line 1097It never calls
createAgreement, and never callsterminateAgreement. A flow is deleted by writing the same agreement data back withflowRate = 0.So the existing
_onlyNotPaused()oncreateAgreementprotected nothing on the streaming path. (It is not dead code — the IDA callscreateAgreementfor index and subscription creation — so the guard is kept, with a corrected comment.)The fix
Because create and delete share one code path, a blanket pause on
updateAgreementDatawould also block shutting malicious streams down — the opposite of what is needed during an incident. The guard therefore compares the new flow rate against the stored one and rejects only an increase:+n > 0+2n > +n+n/2 < +n0 < +ndeleteFlow)0 < +nsignextend(11, ...)sign-extends from bit 95, discarding the timestamp packed at bits 224+ and recovering theint96flow rate. An unset slot reads0, so a fresh flow is+n > 0and is blocked.About # (link your issue here)
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by Sourcery
Enforce pause and blocklist policies across Superfluid agreement operations while preserving the ability to reduce, close, and liquidate existing streams.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment:
Tests:
Chores: